Skip to content

Add ability to compose multi-value Machinery translations#4236

Open
mathjazz wants to merge 24 commits into
mozilla:mainfrom
mathjazz:machinery-compose-multi-value-2886
Open

Add ability to compose multi-value Machinery translations#4236
mathjazz wants to merge 24 commits into
mozilla:mainfrom
mathjazz:machinery-compose-multi-value-2886

Conversation

@mathjazz

Copy link
Copy Markdown
Collaborator

Fix #2886.

@codecov-commenter

codecov-commenter commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 58 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.54%. Comparing base (5ab4cb3) to head (04ddbd0).
⚠️ Report is 3 commits behind head on main.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@eemeli eemeli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at the Python parts only thus far.

Comment thread pontoon/machinery/views.py Outdated
Comment thread pontoon/machinery/views.py Outdated
Comment thread pontoon/machinery/views.py Outdated
Comment thread pontoon/machinery/views.py Outdated
Comment thread pontoon/pretranslation/pretranslate.py Outdated
Comment thread pontoon/pretranslation/pretranslate.py Outdated
@mathjazz

mathjazz commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the comments.

Please note that I've intentionally not flagged anyone for code review yet, because I'd like to first get feedback on the functionality. The code is still deployed to https://pontoon.allizom.org/.

mathjazz added 15 commits June 30, 2026 12:22
For Fluent and MF2-handled formats (Android, Gettext, WebExt, Xcode,
Xliff), the Machinery panel only matched on the first input field
(via `getPlainMessage()`), so attributes and selector variants were never
surfaced.

This mirrors Pretranslation's complex string composition in Machinery,
adding directly-pasteable composed suggestions alongside the
existing results.

More details:

1. Parameterize Pretranslation with mt_provider/mt_service_name/
mt_supported, and move entity-walking into `Pretranslation.walk_entity()`
so Machinery can reuse the composition pipeline.

2. Add `/machinery-composed/` endpoint that walks the entity, looks up
each value in TM, and falls back to the requested MT service for any
remaining value. Returns the composed string + the actual mix of
services used (TM badge + MT badge for hybrid results).

3. Frontend fires composed requests in parallel with the existing
fetches when the entity format can have multiple values. Composed
results dedupe through the existing `addResults()` merge.
on the entity having more than one translatable input, reusing the editor's
field-counting logic.

2. Surface a quality badge. When every value is a 100% TM match, the composed
string is a perfect TM match, so return quality 100 and pass it through to
the panel.

3. Render composed (multi-value) suggestions as labeled fields, the same
representation as the original string panel.
across all input fields, reusing the field-building logic that
is already used by the History panel.

The plain message is recorded as `machinery.translation` so that source
attribution still matches the saved translation on submit.
string isn't suggested back to itself. The composed path didn't, so a
composed TM result could be reconstructed from the entity's own
translation after it was translated.

Add an opt-in `exclude_entity` flag to Pretranslation that excludes the
entity's own TM entries from per-value lookups, and enable it from the
Machinery composed view. A value that can only be served by the entity's
own translation then has no TM match, so a TM-only composition relying
on it is no longer produced. Pretranslation behavior is unchanged.
Re-applying a composed Machinery suggestion (or restoring history)
after editing a field took two clicks: the first did nothing.

Typing updates only CodeMirror's internal doc and EditorResult, not
EditorData.state.fields, so TranslationForm doesn't re-render and each
EditField keeps a stale `defaultValue`. EditField re-syncs its document
only in `useEffect(() => setValue(defaultValue), [defaultValue])`.
distributeEntrySource builds new fields with placeholder handles while
the on-screen editors stay bound, via their React key, to the previous
fields' live handles. The re-applied value for the edited field equals
its stale `defaultValue`, so the effect doesn't fire and the field
isn't updated. A later re-render refreshes `defaultValue`, which is why
the second click works.

Push the distributed values straight into the live handles, matched by
field id, the same way clearEditor does, so one click suffices.
The "Refine using AI" dropdown doesn't work on composed (multi-field/
plural) suggestions: the loader never shows, the refined result never
updates the UI, and copying dumps the raw Fluent source into the first
field. The backend /gpt-transform/ endpoint also refines a single
string, so it can't preserve the entry structure (e.g. returns 2 plural
forms instead of 4).

Hide the dropdown for composed suggestions so they behave like a plain
Google Translate source. Proper composed support is left as a follow-up.
When a suggestion combines multiple sources (e.g. GOOGLE TRANSLATE and
TRANSLATION MEMORY), the source titles ran together with no separator.
…ntity.value and .properties represent a single-pattern or multi-pattern message.
@mathjazz mathjazz force-pushed the machinery-compose-multi-value-2886 branch from e11f61f to 960112e Compare July 1, 2026 09:42
@mathjazz mathjazz requested a review from eemeli July 1, 2026 09:52
@mathjazz

mathjazz commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased and addressed comments.

@flodolo

flodolo commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Could you rebase this? Also, did we remove this code from the dev branch? 🤔

Comment thread pontoon/machinery/tests/test_composed.py Outdated
Comment thread pontoon/machinery/views.py Outdated
Comment on lines +143 to +146
# Only multi-pattern messages — those with multiple properties and/or
# selector variants — have something to compose. A single-pattern message
# composes to the same string the per-leaf machinery already returns, so
# there is nothing extra to show.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not always true; if the source message contains a plural selector, it may have only one pattern, but require multiple patterns in the target locale.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code works as you are describing, but the comment is confusing (also looking at the comment above), because it uses the term "pattern" differently than moz-l10n.

What do you think about replacing the term "pattern" here and in other places with "leaf"? A leaf essentially means the number of input fields shown in the editor.

@eemeli eemeli Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that if the entity.value is representing a Fluent message like

popup-warning-exceeded-message =
    { $popupCount ->
       *[other] { -brand-short-name } prevented this site from opening more than { $popupCount } pop-up windows.
    }

then languages like Slovenian will want to represent it with multiple patterns:

popup-warning-exceeded-message =
    { $popupCount ->
        [one] { -brand-short-name } je strani preprečil, da bi odprla več kot { $popupCount } pojavno okno.
        [two] { -brand-short-name } je strani preprečil, da bi odprla več kot { $popupCount } pojavni okni.
        [few] { -brand-short-name } je strani preprečil, da bi odprla več kot { $popupCount } pojavna okna.
       *[other] { -brand-short-name } je strani preprečil, da bi odprla več kot { $popupCount } pojavnih oken.
    }

even if the unused one variant is left out.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Let me address this.

Comment on lines +183 to +187
response = {
"original": entity.string,
"translation": translation,
"sources": sources_used,
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this API serialising the value+properties, and not returning them as data model values?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with the rest of the machinery API. Every machinery source returns { original, translation } as plain strings, and the Machinery panel sorts, dedups, renders, and copies them uniformly.

And, the frontend already parses full entry sources. On copy, distributeEntrySource calls `parseEntry(format, str)à to spread the leaves across the editor fields.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should work towards (later, separately) transitioning the other machinery API endpoints to also using a data model representation for messages, as we need to get that in any case to validate that what we're getting e.g. from machine translation is valid.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be done as part of KR2: Refactor the TM entry representation around the message data model.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, if you insist. But I do think the PR's current API is trying to fit a square peg in a round hole just because the round hole already exists.

Comment thread pontoon/pretranslation/pretranslate.py Outdated
Comment thread pontoon/pretranslation/pretranslate.py Outdated
Comment thread pontoon/sync/formats/__init__.py Outdated
Comment thread translate/src/context/MachineryTranslations.tsx Outdated
Comment thread translate/src/context/MachineryTranslations.tsx Outdated
Comment on lines +25 to +28
// Set for `/machinery-composed/` results, whose `original` and `translation`
// are full entry sources (Fluent attributes, MF2 variants) rather than plain
// strings — the Machinery panel renders these in a rich, multi-field view.
composed?: boolean;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be simpler to define a new ComposedMachineryTranslation type, as it needs to carry multiple patterns instead of the single pattern of MachineryTranslations?

That would also make it easier to pass the value+properties in the API as JSON data model values, rather than serialising them on the server and re-parsing on the client.

mathjazz added 7 commits July 6, 2026 12:30
value_from_string in pontoon.sync duplicated parse_db_string_to_json. Consolidate into a single parse_source_string_to_json in pontoon.translations (beside
its inverse, serialize_for_db); parse_db_string_to_json now delegates to it, and the key-aware path picks up the MF2 catchall normalization the sync copy was missing.
…cialFormats, which is byte-for-byte identical
…ntity.properties with no parse. Also, document the source-side heuristic.
The composed-machinery gate counted source patterns, so an en-US message with only `*[other]` was treated as single- pattern and skipped — even for locales like Slovenian that need one/two/few/other.

Count target patterns instead: the backend reuses the plural expansion walk_entity() already performs, and the frontend's hasMultipleFields expands plural selectors to the locale's CLDR categories. A single-variant source now composes to a full multi-pattern suggestion for the target locale.
@mathjazz mathjazz requested a review from eemeli July 6, 2026 15:27

@eemeli eemeli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least a couple of the front-end comments from the previous review still need to be addressed too.

Edit: Actually, just #4236 (comment)

Comment on lines +195 to +202
translation = body["translation"]
assert "[one]" in translation
assert "[two]" in translation
assert "[few]" in translation
assert "*[other]" in translation
assert translation.count("TM_popups") == 4
assert body["sources"] == ["translation-memory"]
assert body["quality"] == 100

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be clearer as a single assert body == ... test.

Comment on lines +51 to +56
def parse_db_string_to_json(
res_format: str,
source: str,
) -> tuple[JsonMessage, dict[str, JsonMessage] | None]:
_, value, properties = parse_source_string_to_json(res_format, source)
return value, properties

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should not exist, and we should just refactor the places where it's called to call parse_source_string_to_json instead.

Comment thread pontoon/api/views.py
Comment on lines +534 to +541
key, value, properties = parse_source_string_to_json(fmt, text)
entity = SimpleNamespace(
resource=resource,
string=text,
key=key,
value=value,
properties=properties,
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only non-test code that makes any use of the key value returned by parse_source_string_to_json, and the use it's put to is to set the corresponding value in the fake Entity that's constructed here, which is only used to set the id of the temporary Entry that's passed to set_accesskeys, which ignores it.

So we don't actually need to return it in the first case.

Comment thread pontoon/api/views.py
value=value,
properties=properties,
)
pretranslation = get_pretranslation(entity=entity, locale=locale)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs another TODO/FIXME comment about refactoring so that we don't need to construct the fake Project, Resource, and Entity values to call it.

@eemeli eemeli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, looks like I missed some of the translate/ changes.

Comment on lines +67 to +82
let count = 1;
for (let i = 0; i < msg.sel.length; ++i) {
if (plurals.has(i)) {
count *= Math.max(1, pluralCategories);
} else {
// Non-plural selector (e.g. a gender): keep its distinct source keys.
const keys = new Set(
msg.alt.map((v) => {
const key = v.keys[i];
return typeof key === 'string' ? key : '*';
}),
);
count *= keys.size || 1;
}
}
return count;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unnecessarily accurate. We don't actually care about the pattern count, we only care if it's more than one.

Comment on lines +161 to +166
specialFormats.has(format) &&
hasMultipleFields(
entity.value,
entity.properties,
locale.cldrPlurals.length,
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the early exits in patternCount(), there's no reason to gate this on the format as well.

Suggested change
specialFormats.has(format) &&
hasMultipleFields(
entity.value,
entity.properties,
locale.cldrPlurals.length,
);
hasMultipleFields(
entity.value,
entity.properties,
locale.cldrPlurals.length,
);

@flodolo

flodolo commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

I think I just encountered a bug.

I have a translation for this string

perftools-presets-networking-with-logs-label = Networking with Logs

I have another string with different morphology, same text

profiler-popup-presets-networking-with-logs-label =
    .label = Networking with Logs

Pontoon tries to put the syntax in the field, and shows a 100% match with the syntax in MACHINERY.

Screenshot 2026-07-07 alle 06 25 36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unify TM/MT used by Pretranslation and Machinery

4 participants